animus-cli: bound plugin/daemon tokio worker threads to cgroup CPU quota (not host cpu count) - #342
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Shooksie
left a comment
There was a problem hiding this comment.
This cannot merge as-is: rustfmt is failing, and the branch predates the current rc.33/runtime refactor lineage. Rebase onto current main, keep the cgroup-quota thread calculation isolated from unrelated runtime changes, rerun workspace fmt/clippy/check/tests, and provide Linux cgroup v1/v2 plus unlimited/malformed quota coverage on the rebased head.
There was a problem hiding this comment.
VERDICT: CHANGES REQUESTED
crates/orchestrator-plugin-host/src/cgroup_threads.rs(parse_cgroup_v1) — the new head contains the unformatted declarationpub(crate) fn parse_cgroup_v1(quota_str: &str, period_str: &str) -> Option<usize> {, and the requiredrustfmtcheck is currently failing on head6c339dc76a78abf52794c792d031d606233e0a25. REQUIRED CHANGE: run workspace formatting and commit every formatting change produced for this PR (cargo fmt --all); do not merge while the required rustfmt job is red. Verification: runcargo fmt --all -- --checkand confirm it exits 0, then push and confirm the GitHubrustfmt,cargo-check (workspace),clippy, and Rust dependency guardrail checks all complete successfully.
The implementation is otherwise focused on TASK-628: it introduces one shared cgroup v2/v1 fallback helper, preserves the explicit TOKIO_WORKER_THREADS override, applies the value to the in-repo runtime entry points and child-process seams, and includes useful quota/unlimited/floor parsing coverage.
There was a problem hiding this comment.
VERDICT: CHANGES REQUESTED
crates/orchestrator-plugin-host/src/cgroup_threads.rs(parse_cgroup_v1) — the synchronized head still containspub(crate) fn parse_cgroup_v1(quota_str: &str, period_str: &str) -> Option<usize> {, and both requiredrustfmtcheck runs fail on head1e39b2f8a7bd3602eb304da818aa4cbe8a59be33. REQUIRED CHANGE: runcargo fmt --allfrom the workspace root and commit the formatter's multiline rewrite (plus every other formatter-generated change) toanimus/TASK-628; do not hand-edit only one line or merge a red head. Verification: runcargo fmt --all -- --checkand confirm exit code 0 locally, then push and confirm the GitHubrustfmtcheck completes successfully; also wait for the currently running workspace cargo-check, clippy, smoke-help, and Rust dependency guardrail jobs to finish successfully.
The implementation remains focused and technically well structured: it centralizes cgroup v2/v1 quota parsing, preserves the operator override, applies the computed limit at the relevant in-repo runtime and child-process seams, and includes useful malformed/unlimited/floor coverage.
There was a problem hiding this comment.
VERDICT: CHANGES REQUESTED
crates/orchestrator-plugin-host/src/cgroup_threads.rs(parse_cgroup_v1) — head4ffab8f136a8acf111174e049a964d9c0e1ad27dstill contains the rustfmt-rejected declarationpub(crate) fn parse_cgroup_v1(quota_str: &str, period_str: &str) -> Option<usize> {, and bothrustfmtruns on this synchronized head completed withfailure. REQUIRED CHANGE: runcargo fmt --allfrom the workspace root and commit every formatter-generated rewrite toanimus/TASK-628, including rustfmt’s multiline signature rewrite forparse_cgroup_v1; do not hand-edit only the reported line. Verification: runcargo fmt --all -- --checkand require exit code 0 before pushing, then confirm the GitHubrustfmtcheck is successful and that workspace cargo-check, clippy, smoke-help, and the Rust dependency guardrail all finish successfully on the same head SHA.
The implementation itself remains focused and well aligned with TASK-628: it centralizes cgroup v2/v1 quota parsing, preserves the explicit operator override, applies the computed limit at the relevant in-repo runtime and child-process seams, and includes useful quota, unlimited, malformed, and floor coverage.
There was a problem hiding this comment.
VERDICT: CHANGES REQUESTED
crates/orchestrator-plugin-host/src/cgroup_threads.rs:78—pub(crate) fn parse_cgroup_v1(quota_str: &str, period_str: &str) -> Option<usize> {remains in the unformatted one-line form, and therustfmtcheck is failing on head6b444eb7e678e779a3f0c91032bbc169dc6d93f9. REQUIRED CHANGE: runcargo fmt --allfrom the workspace root and commit every formatter-generated rewrite on the existinganimus/TASK-628branch; do not hand-edit only this signature. Verify withcargo fmt --all -- --check, which must exit 0, and confirm the GitHubrustfmtcheck succeeds on the resulting pushed SHA.
The cgroup-v2/v1 fallback logic, explicit TOKIO_WORKER_THREADS override, runtime-builder integration, and focused regression tests otherwise align well with TASK-628.
There was a problem hiding this comment.
VERDICT: CHANGES REQUESTED
crates/orchestrator-plugin-host/src/cgroup_threads.rs:78— head03cf7507dc95db6edbb1db51d9d8871a6b6f4bcdstill contains the rustfmt-rejected one-line declarationpub(crate) fn parse_cgroup_v1(quota_str: &str, period_str: &str) -> Option<usize> {, and bothrustfmtcheck runs on this synchronized head completed withfailure. REQUIRED CHANGE: from the workspace root, runcargo fmt --alland commit every formatter-generated rewrite to the existinganimus/TASK-628branch, including rustfmt's multiline rewrite of this signature; do not hand-edit only the reported line. Verify withcargo fmt --all -- --checkand require exit code 0 before pushing, then confirm the GitHubrustfmtcheck succeeds on the resulting head SHA and that the remaining workspace cargo-check, clippy, smoke-help, and Rust dependency guardrail jobs finish successfully.
The implementation itself remains focused and well aligned with TASK-628: it centralizes cgroup v2/v1 quota parsing, preserves the explicit TOKIO_WORKER_THREADS override, applies the computed limit at the relevant in-workspace runtime and child-process seams, and includes useful quota, unlimited, malformed, and floor coverage.
There was a problem hiding this comment.
VERDICT: CHANGES REQUESTED
crates/animus-runtime-utils/src/cgroup_threads.rs:78— the current head2db8247072a89b3c7be1edbe1b8d868502a9a99bstill contains the rustfmt-rejected one-line declarationpub(crate) fn parse_cgroup_v1(quota_str: &str, period_str: &str) -> Option<usize> {, and the requiredrustfmtcheck completed withfailure. REQUIRED CHANGE: runcargo fmt --allfrom the workspace root and commit every formatter-generated rewrite onanimus/TASK-628, including rustfmt's multiline rewrite of this signature; do not hand-edit only this line. Verify by runningcargo fmt --all -- --checkand requiring exit code 0 before pushing, then confirm GitHubrustfmtsucceeds on the resulting head SHA. Also wait for cargo-check, clippy, and the dependency guardrail to complete successfully; the currently failing docs-sync check must be green before merge.
The implementation remains focused and technically well aligned with TASK-628: the shared helper preserves the explicit environment override, handles cgroup v2 and v1 quotas with safe fallbacks, is wired into the relevant in-workspace runtime seams, and has useful malformed, unlimited, fractional-quota, and floor coverage.
There was a problem hiding this comment.
VERDICT: CHANGES REQUESTED
-
crates/animus-runtime-utils/src/cgroup_threads.rs:78—pub(crate) fn parse_cgroup_v1(quota_str: &str, period_str: &str) -> Option<usize> {is still not in the canonical rustfmt layout, and therustfmtcheck fails on head887e9351e1fcb2fdcb7fe0c4244c9eb4b09bc17e. REQUIRED CHANGE: runcargo fmt --allon this exact head and commit every formatter-generated rewrite; in particular, let rustfmt split this function signature into its multiline form rather than preserving the current single line. Verify withcargo fmt --all -- --check; it must exit 0, and the GitHubrustfmtcheck must conclude success on the pushed SHA. -
Cargo.toml:2/docs/architecture/full-system-architecture.md(Workspace Inventory) — the workspace now contains 11 members after adding"crates/animus-runtime-utils", while the documentation still says ``Cargo.tomlcurrently declares 10 workspace members.The same stale count remains in `docs/architecture/crate-map.md`, and `scripts/check-doc-sync.sh` requires the count to agree across all guarded documentation, which is why `docs-sync` fails on this head. REQUIRED CHANGE: update the workspace count from 10 to 11 in `docs/architecture/full-system-architecture.md`, `docs/architecture/crate-map.md`, `docs/contributing/development.md`, `docs/design/acp-integration.md`, and `docs/architecture/index.md`; also add `animus-runtime-utils` to the appropriate workspace inventory/crate table so the new shared helper is documented rather than only changing the number. Verify with `bash scripts/check-doc-sync.sh`; it must exit 0 and print `CLI command tree and MCP tool reference are in sync.`, and the GitHub `docs-sync` check must conclude success.
The core approach is otherwise well aligned with TASK-628: the helper is dependency-light, honors TOKIO_WORKER_THREADS first, handles cgroup v2/v1 quota parsing with tests, and keeps the implementation scoped to this repository. Cargo-check, clippy, and the dependency guardrail are still running; all required checks must be green on the same head before merge.
There was a problem hiding this comment.
VERDICT: CHANGES REQUESTED
-
crates/animus-runtime-utils/src/cgroup_threads.rs:78—pub(crate) fn parse_cgroup_v1(quota_str: &str, period_str: &str) -> Option<usize> {is not in the repository's formatter-approved layout, and therustfmtcheck is failing on head887e9351e1fcb2fdcb7fe0c4244c9eb4b09bc17e. REQUIRED CHANGE: runcargo fmt --allon this exact head and commit every formatter-generated rewrite, including the multiline formatting ofparse_cgroup_v1. Verify withcargo fmt --all -- --check; it must exit 0, and the GitHubrustfmtcheck must complete successfully on the pushed SHA. -
docs/architecture/full-system-architecture.md:29,docs/architecture/crate-map.md:3,docs/contributing/development.md:32,docs/design/acp-integration.md(Current Animus Architecture), anddocs/architecture/index.md:3— the new crate makes this an 11-member workspace, but these files still say10 workspace members,10 crates, or10 current workspace members; the workspace inventories also omitanimus-runtime-utils. This exact drift is reflected by the failingdocs-synccheck. REQUIRED CHANGE: change every stale workspace count from 10 to 11 and addanimus-runtime-utilsto the applicable workspace inventory/crate table (and development tree), describing it as the dependency-light cgroup/runtime sizing helper introduced by this PR. Verify withbash scripts/check-doc-sync.sh; it must exit 0, and the GitHubdocs-synccheck must complete successfully on the same pushed SHA.
The implementation is otherwise focused and promising: it centralizes cgroup v2/v1 quota parsing in a dependency-light crate, preserves TOKIO_WORKER_THREADS precedence, includes useful parser edge-case tests, and the workspace cargo-check, clippy, smoke, and dependency-guardrail checks are already green. Because rustfmt and docs-sync remain red, this head must not be merged yet.
887e935 to
fa523bf
Compare
|
Rework complete on exact head |
There was a problem hiding this comment.
VERDICT: APPROVE
The implementation is focused and complete for TASK-628. It introduces a dependency-light helper that honors TOKIO_WORKER_THREADS, then reads cgroup v2 and v1 quotas with a safe system fallback and a floor of one. The daemon/CLI runtime entry points and clean-environment plugin/workflow-runner launch seams consistently apply the computed bound, with parser and propagation regression coverage. Workspace inventory documentation and Cargo metadata are updated without unrelated code churn.
All checks on head fa523bfa1f8b1d9fca760cbee64bd9253088d126 are green: rustfmt, clippy, workspace cargo-check, docs-sync, Rust-only dependency guardrail, smoke-help, and Vercel. The branch is mergeable against main.
What is genuinely good: the shared helper avoids dependency cycles, preserves the operator override, handles malformed/unlimited quotas conservatively, and tests both quota arithmetic and the env propagation paths that previously lost the bound after env_clear().
Dismissed after exact requested rework: rebased to current main, reduced to one focused commit, formatter/docs/workspace/clippy/smoke/guardrail checks all green, and cgroup v1/v2 unlimited/malformed coverage passes.
Automated change for TASK-628 via the Animus coding workflow (deterministic git on a shared ephemeral node).